Skip to content

add & update options#3045

Merged
mercury233 merged 8 commits into
Fluorohydride:masterfrom
mercury233:patch-option
Apr 24, 2026
Merged

add & update options#3045
mercury233 merged 8 commits into
Fluorohydride:masterfrom
mercury233:patch-option

Conversation

@mercury233

@mercury233 mercury233 commented Apr 9, 2026

Copy link
Copy Markdown
Collaborator
image
  • add myclamp utility template to config.h
    std::clamp is avail since C++17, but we are using C++14; also replaced a few manual max expressions with std::max

  • change sound_volume / music_volume from double to int
    reduce the previous ×100 / ÷100 conversions

  • add resize popup menu option
    replaces the old boolean resize_popup_menu with an integer level (0-5); adds a scrollbar in the options UI; ResizeCmdMenu() now reads the level to scale button height

image image image image
  • add resize select window option
    when enabled, card/position select dialog buttons scale with the screen (yScale); when disabled they use a fixed scale (1.2, the previous default for card)

  • add swap yes/no button option
    when enabled, place the "No" button on the left, align with some Japan games

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the options system by adding UI/config toggles for resizing certain dialogs/menus and swapping Yes/No button order, while also simplifying volume handling and introducing a C++14-compatible clamp helper.

Changes:

  • Added new options: resize popup command menu (level 0–5), resize select dialogs, and swap Yes/No button positions.
  • Switched sound_volume / music_volume config values from double (0–1) to int (0–100) and updated audio backends accordingly.
  • Introduced myclamp and replaced several manual min/max bounds checks with myclamp / std::max.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
strings.conf Adds new system strings for the added options.
gframe/sound_manager.h Updates volume setter APIs to accept int (0–100).
gframe/sound_manager.cpp Converts int volume (0–100) to backend float (0–1).
gframe/image_manager.cpp Applies the new “resize select window” option to button texture scaling.
gframe/game.h Adds new config fields and helpers for popup menu button sizing; declares new UI elements.
gframe/game.cpp Adds option UI controls, config load/save entries, button swapping, and command menu resizing.
gframe/event_handler.cpp Handles new checkbox/scrollbar events; updates popup menu layout logic.
gframe/deck_con.cpp Uses myclamp for big-card zoom bounds.
gframe/config.h Adds myclamp helper template.
gframe/client_field.cpp Uses myclamp/std::max to simplify bounds/max computations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gframe/config.h Outdated
Comment thread gframe/game.cpp Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@salix5

salix5 commented Apr 9, 2026

Copy link
Copy Markdown
Collaborator
  • add myclamp utility template to config.h
    std::clamp is avail since C++17, but we are using C++14; also replaced a few manual max expressions with std::max

Please don't.
If you think moving C++17 is unnecessary, using ternary operator is perfectly fine.
Adding functions from C++17 standard library to a project refusing to move to C++17 is self-contradictory.

@mercury233

Copy link
Copy Markdown
Collaborator Author
  • add myclamp utility template to config.h
    std::clamp is avail since C++17, but we are using C++14; also replaced a few manual max expressions with std::max

Please don't. If you think moving C++17 is unnecessary, using ternary operator is perfectly fine. Adding functions from C++17 standard library to a project refusing to move to C++17 is self-contradictory.

clamp wasn't invented by C++17. The need existed first, people implemented it, and it was later standardized. Re-implementing features from newer standards under an older standard is also a common and well-established practice, known as polyfilling.

BTW, while moving to C++17 doesn't seem to have obvious obstacles, it doesn't appear to bring clear benefits for this project at the moment either.

@mercury233 mercury233 added this to the Version 0x1363 milestone Apr 23, 2026
@mercury233 mercury233 merged commit ca74565 into Fluorohydride:master Apr 24, 2026
10 checks passed
@mercury233 mercury233 deleted the patch-option branch April 24, 2026 01:49
@salix5

salix5 commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

It is 2026 now.
C++17 is the standard published about 10 years ago, and the default cppdialect is already changed to c++17 many years

The question is not "Why should ygopro move to c++17?" now.
The question is:
Why should ygopro stay in C++14?
How long would it stay?
Another 10 years ?

@salix5

salix5 commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

By the way, clamp sometimes hide the real problem.
ZoomBigCard does not need clamp in the first place.
The max ratio should be checked before calling this function.
You can click the zoom button 20 times, and you would see why.

@mercury233

Copy link
Copy Markdown
Collaborator Author

I think what we should do is fix the code to be compatible with the new C++ standard, rather than breaking compatibility with the old standard just to use some new features for non-essential functionality.
#2718 (comment)

I still hold the same view.

Upgrading the language standard to C++17 lacks sufficient justification. A small amount of syntactic sugar and a few additions to the standard library have limited significance. As for the fervor with "the standard", I'll pass.

Upgrading the language standard to C++17 is also unnecessary. It appears that mainstream compilers will not abandon C++14 for at least the next 10 years.

Why should ygopro stay in C++14?

No reason to change it.

How long would it stay? Another 10 years?

Stay until there are some reasons. Such as compliers dropping support, great new features in new standard.

If you can prove C++17 or any newer standard have enough benefits, I'll be happy to upgrade.

@salix5

salix5 commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

I think what we should do is fix the code to be compatible with the new C++ standard, rather than breaking compatibility with the old standard just to use some new features for non-essential functionality.
#2718 (comment)

I still hold the same view.

Upgrading the language standard to C++17 lacks sufficient justification. A small amount of syntactic sugar and a few additions to the standard library have limited significance. As for the fervor with "the standard", I'll pass.

​It’s a bit ironic that we are calling C++17 "non-essential" while simultaneously adding manual implementations of C++17 features (like std::clamp) to the project.

​If these features are useful enough to be implemented by hand, they are essential enough to be used from the standard library.

I think it is time to stop polyfilling the 2016s and embrace the baseline of 2026.

Upgrading the language standard to C++17 is also unnecessary. It appears that mainstream compilers will not abandon C++14 for at least the next 10 years.

​In 2026, C++17 is the "common language" of C++ developers.
Using a custom myclamp instead of std::clamp, or std::enable_if instead of if constexpr, makes the code harder for new contributors (if there is one) to read and more tedious to maintain.

If you would like to know, here are some examples

  • if constexpr
    ...unless SFINAE or std::enable_if is your favorite part in this language.

  • Structured Bindings
    ... unless picking items from std::pair or std::tuple is your favorite part.

  • Guaranteed Copy Elision

  • Parallel STL
    Is it possible to add a hand-crafted polyfill for them ?

Why should ygopro stay in C++14?

No reason to change it.

How long would it stay? Another 10 years?

Stay until there are some reasons. Such as compliers dropping support, great new features in new standard.

If you can prove C++17 or any newer standard have enough benefits, I'll be happy to upgrade.

No, I won't.
If you are happy staying in C++14, then feel free to do so.
Fluorohydride/ygopro can stay at C++14 as long as you want.

@mercury233

Copy link
Copy Markdown
Collaborator Author

It’s a bit ironic that we are calling C++17 "non-essential" while simultaneously adding manual implementations of C++17 features (like std::clamp) to the project.

clamp wasn't invented by C++17.

I think it is time to stop polyfilling the 2016s and embrace the baseline of 2026.

One can always abstract common code snippets into functions. The mere fact that a feature exists in a certain standard doesn't mean we must adopt the entire standard if we only need that one piece.

Using a custom myclamp instead of std::clamp, or std::enable_if instead of if constexpr

These are two completely different concepts and shouldn't be lumped together.

...makes the code harder for new contributors (if there is one) to read and more tedious to maintain.

Following that logic, I'd argue we should accelerate our move to C++20 instead. Actually, using fewer language features often makes code easier to understand. As for decreasing writing complexity, AI-assisted completion largely mitigates that.

Using std::enable_if just to implement if constexpr do looks tedious.

if constexpr

If necessary, I'd actually prefer #ifdef over that.

Structured Bindings

Pure syntactic sugar. As far as I remember, there are still many similar refactors unfinished in ocgcore. Using syntactic sugar only in some places can actually make the codebase more inconsistent.

Guaranteed Copy Elision

It's basically handled automatically by compiler optimizations anyway.

Parallel STL

It shouldn't be used at all when dealing with small-scale data.

Fluorohydride/ygopro can stay at C++14 as long as you want.

Fluorohydride/ygopro shouldn't move to C++17 only because you want.

@salix5

salix5 commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

​Your arguments conflate "concept" with "implementation" and ignore the fundamental language guarantees introduced after C++14.

​1. Guaranteed Copy Elision is NOT an optimization
Saying it's "handled by compilers" is technically incorrect. In C++14, RVO is optional, meaning the type must still have a move/copy constructor to be valid. C++17's mandatory elision allows returning non-movable, non-copyable types (like factory-generated resource handles). This is a language rule change, not a compiler heuristic.

​2. #ifdef cannot replace if constexpr
#ifdef operates at the preprocessor level and has zero knowledge of C++ types or template parameters. You cannot use #ifdef to branch logic based on whether a template argument T is an integer or a pointer. if constexpr is for type-based dispatch; preferring #ifdef for this purpose suggests a fundamental misunderstanding of template metaprogramming.

​3. "Syntactic Sugar" vs. Safety
Structured Bindings are not just sugar; they enable const correctness and proper scope management for multiple return values—something std::tie (which requires pre-declaration of mutable variables) cannot do.

​4. The "Small Scale Data" Straw Man
No one is suggesting Parallel STL for sorting 10 elements. The point is having a standardized, vendor-optimized path for performance-critical tasks without reinventing a task-scheduler every time.

​5. Engineering Reality in 2026
You mentioned that "using fewer language features makes code easier to understand." By that logic, we should all be writing C89. Modern features like std::clamp and if constexpr exist precisely to reduce the mental overhead caused by legacy workarounds (SFINAE, manual range checks, etc.).

​Claiming that "AI completion" justifies keeping writing complexity high is a poor engineering standard. We write code for humans to maintain, not for AIs to generate.

​If the project's goal is to remain a time capsule of 2016, then say so. But don't dismiss standardized, safer, and more efficient language features as "limited significance" when you are already manually polyfilling them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants